fix(bin): stop this home's own --resolve-key closes from re-waking the supervisor - #5008
Closed
tiago-peixoto wants to merge 10 commits into
Closed
tiago-peixoto wants to merge 10 commits into
tiago-peixoto wants to merge 10 commits into
Conversation
Self-announced bookkeeping appends now record their exact byte ranges. Later drains and signal scans skip those ranges, so two distinct --resolve-key answers after an OPEN DECISIONS fold do not each wake the supervisor. Worker-authored lines outside that ledger still signal.
…decisions still wake
Keep the home-appends ledger so this home's own --resolve-key answers do not each wake the supervisor, and keep fold-lag from substituting for watcher classification. Take main's multi-line self-announced append so one answer can close several keys in one write. Conflicts resolved in bin/fm-wake-lib.sh, docs/architecture.md, and tests/fm-watch-triage.test.sh.
…ent to its function
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Clear base conflicts on #4907.
Branch fm/fm4885, last known head 0a21cd0.
The change is: stop this home's own --resolve-key answers from each waking the supervisor.
Was checks-green before conflicts. Do not open a second PR. Never merge upstream.
What Changed
bin/fm-classify-lib.sh(state/.<task>.home-appends, v1 + identity header, merged half-open byte ranges, serialized by a sibling.lock), withstatus_home_appends_record/_covers/_rangeshelpers;fm_wake_status_append_self_announcednow records the exact range it appended before touching the watcher marker, andstatus_retire_presentation_taskretires the ledger and its lock at teardown.bin/fm-wake-lib.sh: the marker-only check is nowfm_wake_signal_reported_current, andfm_wake_signal_seen_currentadditionally treats growth past the classified offset as seen when every grown byte is in the owned ledger - so separate--resolve-keyanswers no longer each force a wake, while any foreign or unrecorded line still wakes.fm_wake_print_annotationskeeps using the reported-only predicate, so owned closes still print in the signal annotation and UNREAD STATUS.tests/fm-wake-queue.test.sh,tests/fm-wake-drain-unread-status.test.sh,tests/fm-watch-triage.test.sh, andtests/fm-send-resolve-key.test.sh(repeat answers stay quiet across a regressed classified offset, a folded worker decision with no home append still wakes, owned growth still annotates turn-ended, unreadable logs still read as unreported), and documented the ledger inAGENTS.md,docs/architecture.md, anddocs/scripts.md.Risk Assessment
✅ Low: The change is well-bounded to one new per-task sidecar plus a split wake/presentation predicate, every path I traced fails toward waking (worker bytes always land in an uncovered gap, and identity mismatch, missing ledger, unreadable file, and shrunk file all read as unreported), the captain-facing presentation path is restored byte-identically to base, retirement is complete, the discriminating regression tests fail on base, and the branch still merges cleanly with current origin/main.
Testing
I stood up throwaway firstmate homes in /tmp and drove the supervision loop the way an operator does: a worker opens keyed decisions, the real watcher process wakes and exits, the operator drains and acknowledges, then answers with real
fm-send --resolve-keycalls. The decisive evidence is a live before/after of the actual failure. The watcher captures a status file's classified endpoint at the top of a poll and only commits it after a slow crew-evidence subprocess; an answer written inside that window leaves the seen marker not vouching for the answer's own bytes. Forcing that window (a deliberately slow crew-state probe, which is what the real probe is), base 1b1b6e0 exits withsignal: .../t1.status- the supervisor woken by its own close - and target a3624c9 stays asleep, then still wakes on the worker's nextblocked:line. The same run proves the presentation guard the author was asked for in review round 2: the turn-ended wake's historical annotation still printst1.status: resolved [key=budget]: answered: approved, go ahead. A second driver covers the ordinary path end to end and finishes with a realbin/fm-teardown.shthat leaves nohome-appendsledger or lock behind. A third hammers the new lock with 12 simultaneous realfm-send --resolve-keyprocesses: the ledger stays a single well-formed ascending range inside the log's bounds, every landed close is presented, and nothing is silently lost. While writing that one I hit sends failing with "task metadata could not be locked for final delivery validation"; that reproduces identically on base under the same burst, is fm-send's own fail-closed meta lock rather than anything this change touches, leaves the decision open and tells the operator, so I scoped the scenario to what the change owns instead of reporting it against this PR. This product has no graphical surface - the end-user surfaces are the watcher's reason lines and the drain's rendered sections - so the artifacts are CLI transcripts of those exact surfaces rather than screenshots. Of the four suites the change touches, three ran green in full;tests/fm-watch-triage.test.shwas still crawling at 98 passes with zero failures when I finished, starved by a parallel run of the same suite in another no-mistakes worktree on this host, and every case this change adds or touches in it had already passed. The before/after of the new regression test itself was exercised only as a test-suite run, not against the running product, so it is reported as untested here; the live before/after of the same defect through the real watcher covers that ground. Broad regression across the rest stays with CI.live-inflight-classify-race.shrun against both commits (race-before-after.txt): base exitssignal: .../t1.status, target stays asleep. The driver first asserts the race actually reproduced (class…live-resolve-key-wake.shscenario 4: after two real fm-send --resolve-key answers, the real fm-watch.sh survives a full poll cycle with no wake reason printed and an empty durable .wake-queue (live-…live-resolve-key-wake.shscenario 5 andlive-inflight-classify-race.shstep 8: appendingblocked: need staging credentialsmakes the real watcher exit withsignal: .../t1.statuslive-inflight-classify-race.shstep 7 on target: with the wake suppressed, the turn-end marker wakes the watcher and the drain prints `wake annotation: ... t1.status: resolved [key=budget]: answered…live-resolve-key-wake.shscenario 6: the drain prints bothresolved [key=budget]andresolved [key=vendor]annotations alongside the worker's blockerlive-resolve-key-wake.shscenario 7: a real project clone, worktree and task are torn down withbin/fm-teardown.sh task-x1(exit 0) after a real --resolve-key answer wrote the ledger and a stale `…live-concurrent-answers.shwith 12 concurrent real fm-send processes, 5 consecutive target runs: single merged ascending range inside the log's byte bounds, no torn line, every landed close presente…tests/fm-send-resolve-key.test.sh(copied onto base 1b1b6e0 and on target), which is a test harnes…Evidence: Live before/after: the supervisor woken by its own --resolve-key answer on base, silent on target
Source: Live before/after: the supervisor woken by its own --resolve-key answer on base, silent on target
########## BASE 1b1b6e0 (before the fix) ########## PASS watcher's classified offset regressed to 92, behind the answer's bytes - the race reproduced FAIL the supervisor was WOKEN by its own --resolve-key answer: signal: .../home/state/t1.status base EXIT=1 ########## TARGET a3624c9 (with the fix) ########## PASS watcher's classified offset regressed to 92, behind the answer's bytes - the race reproduced PASS the watcher stayed asleep: this home's own answer did not wake it PASS the worker's turn end woke the supervisor: signal: .../home/state/t1.turn-ended PASS the turn-ended annotation still presents this home's own close PASS the worker's blocked: line woke the supervisor: signal: .../home/state/t1.status target EXIT=0Evidence: Driver: live in-flight-classification race through the real watcher, fm-send and drain
Source: Driver: live in-flight-classification race through the real watcher, fm-send and drain
Evidence: Live supervisor loop on target: two answers, no re-wake, presentation intact, real fm-teardown.sh retires the ledger
Source: Live supervisor loop on target: two answers, no re-wake, presentation intact, real fm-teardown.sh retires the ledger
=== scenario 4 (the fix): neither of this home's own answers wakes the supervisor again PASS watcher stayed asleep through a full poll cycle: no wake reason, empty wake queue === scenario 5 (adversarial): a later worker line on the same task still wakes PASS the worker's blocked: line woke the supervisor: signal: .../home/state/t1.status === scenario 6 (guard): the answers are still shown to the captain, not hidden wake annotation: unread wake-EVENT since last drain ...: t1.status: resolved [key=budget]: answered: approved, go ahead wake annotation: unread wake-EVENT since last drain ...: t1.status: resolved [key=vendor]: answered: go with vendor B PASS both owned closes still printed on the captain-facing surface === scenario 7 (adversarial): a real fm-teardown.sh leaves no orphaned ledger state PASS precondition: the answered task carries a live home-appends ledger fm-teardown.sh exit=0 PASS real fm-teardown.sh removed the per-task ledger and its stale lock remaining task-x1 state: noneEvidence: Driver: live end-to-end supervisor wake loop including real teardown
Source: Driver: live end-to-end supervisor wake loop including real teardown
Evidence: 12 concurrent real --resolve-key answers: ledger stays well formed, every close presented
Source: 12 concurrent real --resolve-key answers: ledger stays well formed, every close presented
Evidence: The new regression test fails on base 1b1b6e0 and passes on target
Source: The new regression test fails on base 1b1b6e0 and passes on target
# The new regression test, run against BASE 1b1b6e0 (change reverted, test copied in) $ tests/fm-send-resolve-key.test.sh ok - fm-send --resolve-key: the answer send itself closes the open decision ok - fm-send --resolve-key: the close never re-wakes its own home, later lines still do not ok - the first --resolve-key answer was left to re-wake this homePipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
⏭️ **Rebase** - skipped
Step was skipped.
bin/fm-classify-lib.sh:1395-status_retire_presentation_taskremoves the new ledger lock withfm_lock_remove_path "$home_appends_lock" 2>/dev/null || true, which discards the result, while the sibling ledger removal on line 1393 propagates failure intorc. Concrete sequence: a process is SIGKILLed while holding a directory-shaped lock and an unexpected file remains inside.<id>.home-appends.lock/;fm_lock_clean_known_filesremoves only the known names,rmdirthen fails, the failure is swallowed, andstatus_retire_presentation_taskreturns 0 while AGENTS.md:145 states both the ledger and its lock are "removed by teardown". The residue is one empty-ish directory in state/ with no owner; the nextfm_lock_try_acquireon that path still reclaims it through ordinary stale-owner recovery, so nothing is incorrect at runtime. Noted only because the round-3 remedy was specifically about exhaustive retirement of this pair; the|| trueis a deliberate match for the pre-existingfm_lock_remove_pathcall sites (bin/fm-wake-lib.sh:936, bin/fm-afk-start.sh:167), so treating it as an accepted tradeoff is reasonable.✅ **Test** - passed
✅ No issues found.
live-inflight-classify-race.shrun against both commits (race-before-after.txt): base exitssignal: .../t1.status, target stays asleep. The driver first asserts the race actually reproduced (class…live-resolve-key-wake.shscenario 4: after two real fm-send --resolve-key answers, the real fm-watch.sh survives a full poll cycle with no wake reason printed and an empty durable .wake-queue (live-…live-resolve-key-wake.shscenario 5 andlive-inflight-classify-race.shstep 8: appendingblocked: need staging credentialsmakes the real watcher exit withsignal: .../t1.statuslive-inflight-classify-race.shstep 7 on target: with the wake suppressed, the turn-end marker wakes the watcher and the drain prints `wake annotation: ... t1.status: resolved [key=budget]: answered…live-resolve-key-wake.shscenario 6: the drain prints bothresolved [key=budget]andresolved [key=vendor]annotations alongside the worker's blockerlive-resolve-key-wake.shscenario 7: a real project clone, worktree and task are torn down withbin/fm-teardown.sh task-x1(exit 0) after a real --resolve-key answer wrote the ledger and a stale `…live-concurrent-answers.shwith 12 concurrent real fm-send processes, 5 consecutive target runs: single merged ascending range inside the log's byte bounds, no torn line, every landed close presente…tests/fm-send-resolve-key.test.sh(copied onto base 1b1b6e0 and on target), which is a test harnes…FM_LIVE_ROOT=<target> live-inflight-classify-race.shandFM_LIVE_ROOT=<base 1b1b6e0 export> live-inflight-classify-race.sh- the live before/after race through the real fm-watch.sh, fm-send.sh and fm-wake-drain.shFM_LIVE_ROOT=<target> live-resolve-key-wake.sh- the live supervisor loop: worker decisions wake once, two real --resolve-key answers, no re-wake, worker blocker still wakes, realbin/fm-teardown.sh task-x1retires the ledgerFM_LIVE_ROOT=<target> FM_CONC_N=12 live-concurrent-answers.sh(5 consecutive runs) plus 6 base runs for comparison - concurrent real fm-send --resolve-key processes against one tasktests/fm-send-resolve-key.test.shon target (23 pass) and the same file copied onto base 1b1b6e0, wheretest_separate_resolve_key_answers_do_not_rewakefailstests/fm-wake-queue.test.sh- includes test_separate_self_announced_answers_after_fold_are_owned, test_unreadable_status_is_not_owned, test_folded_worker_resolved_is_not_owned_lag, test_owned_growth_still_annotates_turn_endedtests/fm-wake-drain-unread-status.test.sh- includes test_self_announced_pending_reply_close_still_surfaces and the retired-task-id ledger/lock removal casetests/fm-watch-triage.test.sh- the change's cases (test_folded_worker_decision_without_home_append_still_wakes, test_separate_self_announced_answers_after_fold_wake_once, the self-announced-close cases) all passed early; the rest of the file was still running at 98 passes / 0 failures when I finishedAGENTS.md:145- Judgment call, left as-is: the clause "presentation is unaffected, so both the signal annotation and UNREAD STATUS still print those lines" in the AGENTS.md state inventory restates the same contract that docs/architecture.md:96 states ("it never removes a line from presentation, so both that annotation and the UNREAD STATUS section still print this home's own bookkeeping closes"). Both copies are currently true, so neither is stale. I judged this legitimate rather than duplication needing consolidation: docs/documentation-audiences.md classifies AGENTS.md as agent-runtime (an operating contract a Firstmate agent reads directly) and architecture.md as maintainer-architecture, and the surrounding AGENTS.md inventory entries follow the same pattern of appending one operative clause to each file's description. Flagging it only so the divergence risk is visible: if a future change alters whether the ledger touches presentation, both lines must move together, and architecture.md:96 is the owner.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.